Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable tabby to print debug logs #812

Closed
wants to merge 7 commits into from

Conversation

costanzo
Copy link
Contributor

Add --enable-debug flag so tabby will print debug logs.

@wsxiaoys
Copy link
Member

wsxiaoys commented Nov 17, 2023

You could enable debug log by setting

RUST_LOG=debug tabby ...

Maybe just add this to FAQ?

@wsxiaoys wsxiaoys marked this pull request as draft November 18, 2023 06:12
@costanzo
Copy link
Contributor Author

I have tried the RUST_LOG=debug eviroment flag. Indeed some modules' debug logs are enabled but the tabby debug log itself is not enabled. Because it forced to INFO here crates/tabby/src/main.rs:

...
.add_directive("tabby=info".parse().unwrap())
...

I am not sure why it is set INFO here but maybe we can remove that and accept the RUST_LOG environment flag to control?

@costanzo costanzo marked this pull request as ready for review November 20, 2023 05:48
@costanzo costanzo changed the title feat: add enable debug log flag feat: enable tabby to print debug logs Nov 20, 2023
@wsxiaoys
Copy link
Member

wsxiaoys commented Nov 20, 2023

I am not sure why it is set INFO here but maybe we can remove that and accept the RUST_LOG environment flag to control?

The goal is to enable default info-level logging for the Tabby package. Perhaps we can implement a check to prevent the acceptance of a more verbose environment variable override, such as RUST_LOG=tabby=debug.

@costanzo
Copy link
Contributor Author

Oh, I just realized the default tracing log level is ERROR not INFO if RUST_LOG is not set.

Perhaps we can implement a check to prevent the acceptance of a more verbose environment variable override, such as RUST_LOG=tabby=debug

Why do we need to avoid a verbose environment variable override. I mean we definitely need to print debug logs in Tabby package, right?

@wsxiaoys
Copy link
Member

Perhaps we can implement a check to prevent the acceptance of a more verbose environment variable override, such as RUST_LOG=tabby=debug

Why do we need to avoid a verbose environment variable override. I mean we definitely need to print debug logs in Tabby package, right?

To avoid pretenting the acceptance :)

Based on https://docs.rs/tracing-subscriber/latest/src/tracing_subscriber/filter/env/builder.rs.html#69

A proper implementation would be

let filter = EnvFilter::builder()
     .with_default_directive(LevelFilter::ERROR.into())
     .parse_lossy("tabby=info")
     ...
     .from_env_lossy()

@wsxiaoys wsxiaoys marked this pull request as draft November 20, 2023 06:53
@costanzo
Copy link
Contributor Author

Hi @wsxiaoys if I am understanding correctly, we want to set the default log level to be error, and set default tabby log level to be info. At the same time we want the envrionment log settings to be able to override the default tabby log level?

I check the tracing-subscriber doc and find there is no way of doing that.

parse_lossy method will return an EnvFilter, which will no longer parse env.

So I just set default directive to be LevelFilter::INFO which can enable Tabby pacakge logging. And info log in other packages is helpful for various purposes.

@costanzo costanzo marked this pull request as ready for review November 20, 2023 14:55
@wsxiaoys
Copy link
Member

I think it's still doable - though you might need dig into the implementation of EnvFilter a bit.

@wsxiaoys wsxiaoys marked this pull request as draft November 24, 2023 01:52
@wsxiaoys
Copy link
Member

wsxiaoys commented Mar 6, 2024

This is done recently:

let mut dirs = "tabby=info,axum_tracing_opentelemetry=info,otel=debug".to_owned();

@wsxiaoys wsxiaoys closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants